Closed Bug 713577 Opened 13 years ago Closed 13 years ago

Clang Static Analysis: Operand in multiplication is garbage value in content/svg/content/src/nsSVGFilters.cpp

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: decoder, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

The following (almost identical) reports have been generated by static analysis using Clang: http://users.own-hero.net/~decoder/mozilla/report/2011-12-21-1/report-Nd3e7L.html#EndPath http://users.own-hero.net/~decoder/mozilla/report/2011-12-21-1/report-yCjuGu.html#EndPath http://users.own-hero.net/~decoder/mozilla/report/2011-12-21-1/report-R2SqdD.html#EndPath It would be good if someone familiar with the particular code could check if - this is really a bug or a false positive - and/or if it makes sense to adjust the code (even if there is not a real bug present, e.g. by adding a missing initialization). In these particular reports, the problems seems to be that depending on the value of | values.Length(); |, one of the multiplications later may be using uninitialized/garbage values. The length assumed by the analysis can be seen in line 1203, depending on how many times the loop is executed.
The code ensures that values.Length() == 20 and that all 20 entries of colorMatrix are initialized. That's the loop on lines 1203 and 1204. The loop condition on line 1276 ensures the following invariants: 1) 0 <= i < 4 2) row = 5*i. So the maximal value of |row| is 15. Hence row+3 and row+4 are at most 18 and 19, which are inside the initialized colorMatrix array (the last two entries of it, in fact). So this looks like a false-positive to me at first glance. I really don't know how to make clang not flag this offhand short of changing the line 1276 loop condition to compare row to 20 instead of i to 4, maybe? It may be more worthwhile to see whether the clang bug can be fixed.
First of all, thanks for looking into this :) (In reply to Boris Zbarsky (:bz) from comment #1) > The code ensures that values.Length() == 20 Where is that ensured? It's probably just that what's being missed by the analysis, because it assumes | values.Length() | to me smaller (and in that case, the result would be correct).
The value of values.Length() is checked somewhat higher up in a switch statement. If type == SVG_FECOLORMATRIX_TYPE_MATRIX then we have this: http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGFilters.cpp#1200 All 20 values in the colorMatrix are always initialised. And the index never exceeds 19 i < 4 therefore row < 15 and so row + 4 < 19
Attached patch hackSplinter Review
It looks like the problem is that it is not figuring out that the two calls to Length return the same value. The attached patch hides the warning. I will try figure out if this can be improved on the clang side.
Doesn't seem valid as an SVG bug.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: